Skip to content

Code review sweep (run 24974154176)#18333

Merged
trask merged 6 commits into
mainfrom
otelbot/code-review-sweep-24974154176
Apr 27, 2026
Merged

Code review sweep (run 24974154176)#18333
trask merged 6 commits into
mainfrom
otelbot/code-review-sweep-24974154176

Conversation

@otelbot

@otelbot otelbot Bot commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Automated code review sweep walked the following modules in order
and stopped after accumulating at least 10 modified files:

  • jaxws-metro-2.2:javaagent
  • jboss-logmanager-appender-1.1:javaagent
  • jboss-logmanager-mdc-1.1:javaagent
  • jdbc:bootstrap
  • jdbc:javaagent
  • jdbc:library

Module: jaxws-metro-2.2:javaagent

Module path: instrumentation/jaxws/jaxws-metro-2.2/javaagent

Summary

Applied four safe review fixes in jaxws-metro-2.2 javaagent: renamed one collaborator field to match the style guide and reduced visibility on three package-local implementation types/members per the minimal-visibility rule. metadata.yaml already matched the module's controller-telemetry config usage, so no metadata change was needed.

Applied Changes

Style

File: MetroHelper.java:21
Change: Renamed `SPAN_NAME_UPDATER` to `spanNameUpdater` and updated its call site.
Reason: The style guide reserves uppercase names for constants and stable identifiers; runtime-created collaborator objects should use lower camel case.

File: MetroRequest.java:12
Change: Reduced `MetroRequest` and its package-only constructor/accessors from `public` to package-private.
Reason: The style guide requires minimal necessary visibility, and this request wrapper is only used inside the `metro` package.

File: MetroServerSpanNameUpdater.java:42
Change: Reduced the constructor and `updateServerSpanName(...)` from `public` to package-private.
Reason: The minimal-visibility rule applies here because `MetroServerSpanNameUpdater` is a package-local helper used only by `MetroHelper`.

File: MetroSingletons.java:12
Change: Reduced `MetroSingletons` and its `instrumenter()` accessor from `public` to package-private.
Reason: The singleton holder is an internal javaagent implementation detail with only package-local callers, so broader visibility was unnecessary under the style guide.

Module: jboss-logmanager-appender-1.1:javaagent

Module path: instrumentation/jboss-logmanager/jboss-logmanager-appender-1.1/javaagent

Summary

Applied one safe review fix in LoggingEventMapper and left one build-task wiring issue unresolved because the repository review rules mark testExperimental task creation as non-auto-fix.

Applied Changes

Style

File: LoggingEventMapper.java:35
Change: Reordered `captureExperimentalAttributes` so all static fields are declared before instance fields in `LoggingEventMapper`.
Reason: The style guide's class-organization rule requires static fields to appear before instance fields; this was a safe, behavior-preserving cleanup.

Unresolved Items

File: build.gradle.kts
Reason: Experimental log flags are applied to every `Test` task via `tasks.withType<Test>().configureEach { jvmArgs(...) }`. The review rules require isolating this kind of coverage in a dedicated `testExperimental` task, but they explicitly mark that change as `Do not auto-fix`; the next action is to add a separate experimental test task and move these flags there.

Module: jboss-logmanager-mdc-1.1:javaagent

Module path: instrumentation/jboss-logmanager/jboss-logmanager-mdc-1.1/javaagent

Summary

Reviewed all files under instrumentation/jboss-logmanager/jboss-logmanager-mdc-1.1/javaagent, validated the sibling metadata.yaml against in-module config usage, and applied one safe test-only cleanup aligned with the repository's AssertJ guidance. No additional deterministic fixes were needed.

Applied Changes

[Testing]

File: JbossLogmanagerMdcTest.java:61
Change: Replaced manual indexed log-message assertions with ordered AssertJ collection assertions and reused local `ExtLogRecord` variables for the per-record MDC checks.
Reason: `testing-general-patterns.md` prefers AssertJ idiomatic simplifications such as `extracting(...).containsExactly(...)` over repetitive `list.get(i)` assertions, which keeps the test clearer while preserving behavior.

Module: jdbc:bootstrap

Module path: instrumentation/jdbc/bootstrap

Summary

Reviewed instrumentation/jdbc/bootstrap and validated the JDBC module metadata.yaml; no safe repository-guideline fixes were needed in the only tracked file, build.gradle.kts.

Applied Changes

No safe automated changes were applied.

Module: jdbc:javaagent

Module path: instrumentation/jdbc/javaagent

Summary

Applied 3 safe review fixes in instrumentation/jdbc/javaagent: added missing @Nullable annotations to concrete null-returning helpers and renamed one prepared-statement advice method to match its actual behavior. instrumentation/jdbc/metadata.yaml usage was reviewed and did not need changes.

Applied Changes

Style

File: ConnectionInstrumentation.java:173
Change: Added `@Nullable` to `TransactionAdvice.onEnter()`.
Reason: `TransactionAdvice.onEnter()` returns `null` on wrapper/skip paths, and the repository nullability rule requires annotating methods that concretely return `null`.

File: JdbcAdviceScope.java:77
Change: Added `@Nullable` to `createBatchRequest(...)`.
Reason: `createBatchRequest(...)` can return `null` when no prepared SQL or batch metadata is available, so the nullability guideline requires declaring the nullable return explicitly.

Javaagent

File: PreparedStatementInstrumentation.java:242
Change: Renamed `ClearParametersAdvice.clearBatch(...)` to `clearParameters(...)`.
Reason: The method instruments `clearParameters()`, and the javaagent advice guidance favors names that match the advice behavior instead of leaving a confusing copy/paste name.

Unresolved Items

File: build.gradle.kts
Reason: `metadataConfig` is already used in this module, but several non-default test tasks with task-specific `jvmArgs(...)` still lack a matching `metadataConfig` entry. `gradle-conventions.md` says existing `metadataConfig` usage should cover each non-default task, and the review instructions explicitly say not to auto-add `collectMetadata`/`metadataConfig` during review.

Module: jdbc:library

Module path: instrumentation/jdbc/library

Summary

Applied two safe @Nullable contract fixes in instrumentation/jdbc/library for wrapper APIs that already receive null from in-repo callers. One metadata review item remains unresolved because it touches deprecated config exposure rather than a behavior-safe source fix.

Applied Changes

Style

File: OpenTelemetryDriver.java:243
Change: Added `@Nullable` to the `Properties info` parameter on `connect()` and `getPropertyInfo()`.
Reason: The `Nullability Correctness` rule says parameters should be annotated when a concrete caller can pass `null`; this module's tests call both driver methods with `null` `Properties`.

File: OpenTelemetryDataSource.java:91
Change: Added `@Nullable` to the `username` and `password` parameters on `getConnection(String, String)`.
Reason: The `Nullability Correctness` rule requires `@Nullable` when callers actually pass `null`, and `OpenTelemetryDataSourceTest` exercises `getConnection(null, null)` through this wrapper.

Unresolved Items

File: metadata.yaml
Reason: `OpenTelemetryDriver` still reads the deprecated fallback property `otel.instrumentation.common.db.experimental.sqlcommenter.enabled`, but `metadata.yaml` does not document it; adding or removing that deprecated config path needs a broader deprecation/docs decision rather than a mechanical review fix.


Download code review diagnostics

otelbot Bot added 5 commits April 27, 2026 02:56
Automated code review of instrumentation/jaxws/jaxws-metro-2.2/javaagent.
Automated code review of instrumentation/jboss-logmanager/jboss-logmanager-appender-1.1/javaagent.
Automated code review of instrumentation/jboss-logmanager/jboss-logmanager-mdc-1.1/javaagent.
Automated code review of instrumentation/jdbc/javaagent.
Automated code review of instrumentation/jdbc/library.
@otelbot otelbot Bot requested a review from a team as a code owner April 27, 2026 03:19
@otelbot-java-instrumentation otelbot-java-instrumentation Bot added the test native This label can be applied to PRs to trigger them to run native tests label Apr 27, 2026
@trask trask merged commit e2ec96f into main Apr 27, 2026
96 checks passed
@trask trask deleted the otelbot/code-review-sweep-24974154176 branch April 27, 2026 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module cleanup test native This label can be applied to PRs to trigger them to run native tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant